Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per Particle Logger #4359

Merged
merged 11 commits into from
Jan 10, 2023
Merged

Conversation

PDoakORNL
Copy link
Contributor

Proposed changes

This PR adds per particle Hamiltonian reporting to estimators through the entire application and provides the proof of concept particle Hamiltonian logger. The logger is not optimized for performance nor is the output optimal for actual use. If anyone would like to improve it further they should feel free.

More complete documentation of the Estimator architecture will be added in future PR's.

What type(s) of changes does this code introduce?

Delete the items that do not apply

  • New feature
  • Build related changes
  • Testing changes (e.g. new unit/integration/performance tests)
  • Documentation changes

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

LECONTE

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes This PR is up to date with current the current state of 'develop'
  • Yes Code added or changed in the PR has been clang-formatted
  • Yes This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • Yes Documentation has been added (if appropriate)

better doc re: input class copy constructors

remove InputSection changes
@PDoakORNL PDoakORNL force-pushed the per_particle_logger_with_feeling branch from d47c69d to 5a34c19 Compare December 9, 2022 15:53
@@ -104,6 +108,8 @@ class Crowd
const MultiWalkerDispatchers& dispatchers_;

private:
/// For some output purposes its useful if a crowd has an identifier for it on a rank.
const int crowd_id_ = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point me a use case? Such state usually concerns me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reconsidering this state, I thought it was necessary but now I think it is just lazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to eliminate the crowd_id_ and simply take the Walker.ID seriously. This is a more likely desired for a per particle log anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waker.ID is as bad as crowd_id_. It was not taken seriously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it requires less disturbance to the code. To log per particle per walker hamiltonian components there must be some identification of the walkers. I added unit testing of the walker ids.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

walker ID is global (across all MPI ranks), very hard to keep track of. You can create some local data structure for the indexing, it is far better than replying on an global ID.

@prckent
Copy link
Contributor

prckent commented Dec 19, 2022

I would like to break the symmetry here and get this activity moving again, not only this PR. Peter and I discussed this last week.

First of all, since this PR doesn't break existing functionality, the general guidance about code moving in a good direction being mergeable applies.

Second, one of the use cases for this level of logging is to help diagnose a failed run, at least on "replay" using the same seed and run configuration. In this case having a unique ID for each walker would be very useful, so this is what we should work towards. There is no question that we would have used the capability for this purpose several times in the last year.

In VMC the IDs are trivial, since the walker count is constant. In DMC we need e.g. an incrementing counter and to make sure we can count 100s millions of walkers. Since the walker creation/destruction logic is global this is also not difficult. A new ID is only needed when new walkers are created. The required state information is a single (long?) integer. This can be worked towards in subsequent PRs.

As with general use of the per particle logger, we are concerned with functionality and achievable insight, not straight line speed.

src/Estimators/EstimatorManagerCrowd.cpp Outdated Show resolved Hide resolved
@@ -48,6 +48,9 @@ class StructFact : public QMCTraits
/** Constructor - copy ParticleSet and init. k-shells
* @param lattice long range box
* @param kc cutoff for k
*
* At least in the batched version Structure factor is _NOT_ valid
* after construction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you are trying to describe an issue. Could you put it as an issue in github?

src/QMCDrivers/Crowd.h Show resolved Hide resolved
src/QMCDrivers/QMCDriverNew.cpp Outdated Show resolved Hide resolved
src/Utilities/for_testing/NativeInitializerPrint.hpp Outdated Show resolved Hide resolved
@ye-luo
Copy link
Contributor

ye-luo commented Dec 19, 2022

I would like to break the symmetry here and get this activity moving again, not only this PR. Peter and I discussed this last week.

First of all, since this PR doesn't break existing functionality, the general guidance about code moving in a good direction being mergeable applies.

I think we can make this PR moving with minor changes.

Second, one of the use cases for this level of logging is to help diagnose a failed run, at least on "replay" using the same seed and run configuration. In this case having a unique ID for each walker would be very useful, so this is what we should work towards. There is no question that we would have used the capability for this purpose several times in the last year.

Indexing is unavoidable when recording into a big buffer/file. What I'm strongly against is to keep it as a state of walker. Indexing should be short lived whenever possible. Each element should avoid remembering its index, only the object handling a set of elements deal with indexing in restricted scopes.

In VMC the IDs are trivial, since the walker count is constant. In DMC we need e.g. an incrementing counter and to make sure we can count 100s millions of walkers. Since the walker creation/destruction logic is global this is also not difficult. A new ID is only needed when new walkers are created. The required state information is a single (long?) integer. This can be worked towards in subsequent PRs.

Creating new ID means the ID of other walkers can be out-dated. This requires global synchronization.

As with general use of the per particle logger, we are concerned with functionality and achievable insight, not straight line speed.

@prckent
Copy link
Contributor

prckent commented Dec 20, 2022

Creating new ID means the ID of other walkers can be out-dated. This requires global synchronization.

The ideas is that walkers have unique IDs for their entire lifetime.
If the ID is generated based on uniquely incrementing counter(s) there is no issue with them becoming out of date.
We already have a global sync to decide if to destroy or create walkers, so the IDs can be set or inferred then.
Something to work towards.

@ye-luo
Copy link
Contributor

ye-luo commented Dec 20, 2022

I still need an answer for how walker id is used for in this PR and why this is necessity. I don't mean to block this PR on this but the current use needs to be documented. How do we want that in the future will be a separate topic.

@prckent
Copy link
Contributor

prckent commented Dec 20, 2022

Fair point Ye.

For future walker ID/numbering discussions: if we don't mind not having them numbered consecutively (I don't), we could just number them (walker_generation*no_mpi_tasks+my_mpi_task_no); this needs no coordination at all since the IDs can be generated at the mpi task level (I assume we will take all these decisions at this level - I don't think anything is needed at the crowd level). Then the only state needed is at the task level. This then leaves the question as to whether we want to save it for restarts...

ye-luo
ye-luo previously approved these changes Jan 4, 2023
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: there is still no documentation about walker ID in this PR.

@ye-luo
Copy link
Contributor

ye-luo commented Jan 4, 2023

Test this please

@ye-luo ye-luo enabled auto-merge January 4, 2023 19:51
@PDoakORNL
Copy link
Contributor Author

test this please

@ye-luo
Copy link
Contributor

ye-luo commented Jan 10, 2023

Test this please

@ye-luo ye-luo merged commit c17c65a into QMCPACK:develop Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants